Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix calling HammerDB added to PATH #601

Closed
wants to merge 2 commits into from

Conversation

akantak
Copy link

@akantak akantak commented Sep 21, 2023

Fixing #597

@akantak akantak requested a review from a team as a code owner September 21, 2023 10:40
@akantak akantak marked this pull request as draft September 21, 2023 10:40
@akantak
Copy link
Author

akantak commented Sep 21, 2023

discussion happens in #597

Copy link
Contributor

@sm-shaw sm-shaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like its moving in the right direction but needs a bit more debugging and testing. If I try and run hammerdb from another directory, I get:
Error in startup script: couldn't read file "./HammerDB-4.8-600/hammerdb": no such file or directory
This is because it is trying to execute the absolute path rather than the relative one.
exec wish8.6 -file $0 ${1+"$@"}

So this looks like it needs the same change as the agent e.g.
exec .././bin/tclsh8.6 ${0##*/} ${1+"$@"}

However, there are other tests I did that get broken with these changes such as running the automated scripts such as:

./scripts/tcl/maria/tprocc/maria_tprocc.sh
BUILD HAMMERDB SCHEMA
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-
can't read "LD_LIBRARY_PATH": no such variable
    while executing
"export LD_LIBRARY_PATH="./lib:$LD_LIBRARY_PATH""
    (file "hammerdbcli" line 6)
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-
RUN HAMMERDB TEST
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-
can't read "LD_LIBRARY_PATH": no such variable
    while executing
"export LD_LIBRARY_PATH="./lib:$LD_LIBRARY_PATH""
    (file "hammerdbcli" line 6)
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-
DROP HAMMERDB SCHEMA
can't read "LD_LIBRARY_PATH": no such variable
    while executing
"export LD_LIBRARY_PATH="./lib:$LD_LIBRARY_PATH""
    (file "hammerdbcli" line 6)
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-
HAMMERDB RESULT
can't read "LD_LIBRARY_PATH": no such variable
    while executing
"export LD_LIBRARY_PATH="./lib:$LD_LIBRARY_PATH""
    (file "hammerdbcli" line 6)

So it needs testing with the hammerdbcli auto argument with both Tcl and Python options.
There is also an auto argument to the GUI that will load a file and run the autopilot that needs testing as well.

It needs testing from local and remote as another error I saw running locally was the following:

./hammerdb
While loading module "tkpath"...
can't find package tkpath
    while executing
"package require $m "

On Linux a quick hack showed something like this worked a bit better, however I cannot guarantee it satisifes all test conditions:

#!/bin/sh
########################################################################
# \
cd "$(dirname "$0")"
# \
export LD_LIBRARY_PATH="${0##*/}/lib:./lib:$LD_LIBRARY_PATH"
# \
export PATH="${0##*/}/bin:./bin:$PATH"
# \
exec wish8.6 -file ${0##*/} ${1+"$@"}
# \
exit
########################################################################

On Windows a basic test worked, however I have not tested all scenarios such as running the scripts from Powershell.

So if this has been thoroughly tested then the idea is good, however we need to be absolutely sure that it doesn't break some of the functionality inadvertently as this is the reason it has not been done already as changing to the home directory before running we know for certain every test is passed, so we need to have the same confidence with these changes.

@akantak
Copy link
Author

akantak commented Sep 22, 2023

@sm-shaw Yes, I'm aware of it, that's why it is marked as a draft, to present the flaws in that approach. The relative paths are an issue when using cd at the beginning of a script. That's why I continued a discussion (maybe incorrectly) at #597 on which option to choose from the one that I shared. I will be back on 02.10 to continue the discussion.

@akantak
Copy link
Author

akantak commented Nov 8, 2023

will continue discussion here:
#597
for now, due to lack of my time, closing this PR as there were flaws discovered in presented solution.

@akantak akantak closed this Nov 8, 2023
@akantak akantak deleted the fix-path-executables branch November 8, 2023 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants